Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#17] apply review page #33

Merged
merged 16 commits into from
Jul 22, 2022
Merged

[#17] apply review page #33

merged 16 commits into from
Jul 22, 2022

Conversation

hanseulhee
Copy link
Owner

설명

review page를 개발하였습니다.

중점적으로 봐줬으면 좋을 부분

처음해보는 부분이라 혹시 잘못된 곳이 있다면 알려주심 감사하겠습니다 ... 😵‍💫🥲

게시글은 후에 수정할 계획입니다

@hanseulhee hanseulhee requested a review from hyesungoh as a code owner July 16, 2022 12:23
@hanseulhee hanseulhee self-assigned this Jul 16, 2022
Copy link

@hyesungoh hyesungoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍 👍 👍 👍 👍 👍 👍

궁금하고 권장되는 변경 사항만 몇 개 달아봤어요!

정적 페이지를 생성하시는 것이 처음이면 많이 어려우셨을 텐데 고생하셨어요 💯 🥇

Comment on lines 5 to 7
<div css={bgWrapper}>
<div css={sizeWrapper}>
<div css={colorWrapper}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분에 대해 main, section, article 등의 시맨틱 태그를 적절히 사용하면 괜찮지 않을까요!

import { Link } from "@nextui-org/react";
import { PostMarkdown } from "util/getMarkdownBySlug";

export interface PostListItemProps extends PostMarkdown {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 파일에서는 이 인터페이스가 사용되고 있지 않은데, 여기서 선언한 이유가 있을까요?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

착각했네용 😱

Comment on lines 7 to 8
function PostCard(props) {
const post = props;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 바인딩하는 것보다 아래처럼 가져갈 수도 있을 거 같아요!!!

Suggested change
function PostCard(props) {
const post = props;
function PostCard(post) {

return (
<article css={cardWrapper}>
<div css={contentWrapper}>
<Link href={`/Review/${post?.slug}`}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Link href={`/Review/${post?.slug}`}>
<Link href={`/Review/${post.slug}`}>

slug가 있어야만 동작하는게 맞지 않을까요?

없는 경우가 있다면 따로 Link 태그가 아니도록 핸들링을 하면 될 거 같아요!

Comment on lines +8 to +14
const file = await unified()
.use(remarkParse)
.use(remarkRehype)
.use(rehypeSanitize)
.use(rehypeStringify)
.process(markdown);
return file.toString();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍 👍 👍

Comment on lines +6 to +7
export const getPostBySlug = (slug: string, fields: string[]) =>
getMarkdownBySlug(postsDirectory, slug, fields) as PostMarkdown;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 함수가 하는 역할은 getMarkdownBySlugpostsDirectory를 주입하는 것으로 보여요

그럼 getMarkdownBySlug에서 기본적으로 사용하는 방향으로 하면 해당 함수는 필요없을 수도 있지 않을까요??

Comment on lines 30 to 42
if (field === "slug") {
items[field] = realSlug;
}

if (field === "content") {
items[field] = content;
}

if (field === "tags" && data[field]) {
items[field] = data[field].split("");
} else if (data[field]) {
items[field] = data[field];
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기는 switch 문을 사용하거나 else if로 사용하는 것이 조금 더 읽기 쉬워질 거 같아요!

추가적으로 else에 대해 에러 처리한다거나, undefined를 추가하는 형식으로도 접근할 수 있을 거 같아요!

package.json Outdated
"framer-motion": "^6.3.2",
"next": "^12.1.6",
"gray-matter": "^4.0.3",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 gray-matter 라이브러리는 마크다운 파일의 상단에 있는 카테고리를 반환하는 기능을 해요.

즉, 서버 사이드에서 (빌드 타임) 실행되기 때문에 devDependencies로 내려도 될 거 같아요 👍

next.config.js Outdated
Comment on lines 8 to 16
webpack: (config) => {
config.module.rules.push({
test: /\.md$/,
use: 'raw-loader',
})
return config
},
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 웹팩 설정은 무엇을 의미하나요?

서버 사이드에서만 마크다운 파일을 참조하기 때문에 로더는 필요 없지 않을까? 라고 생각되는데 경험을 공유해주시면 좋을 거 같아요!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그 점을 고려하지 못했네요 ...! 수정하는 게 맞을 것 같습니다 ㅎㅎ

@hanseulhee hanseulhee merged commit 04ecb11 into main Jul 22, 2022
@hanseulhee hanseulhee linked an issue Jul 22, 2022 that may be closed by this pull request
2 tasks
@hanseulhee hanseulhee deleted the feat/reviewPage branch August 30, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blog page 개발
2 participants